Fix bookmark favicon selection in oEmbed fallback#28939
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/services/oembed/oembed-service.js (1)
568-568: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the undefined-type fallback.
This fixes the right contract, but the supplied tests only cover the two halves separately (
fetchOembedDataFromUrl()returning bookmark data, andfetchBookmarkData(..., 'bookmark')preferring the standard favicon). A single test for the exact!typefallback path would lock this bug down.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/services/oembed/oembed-service.js` at line 568, Add a regression test for the exact undefined-type fallback path in oembed-service.js: the current coverage splits the behavior across fetchOembedDataFromUrl() and fetchBookmarkData(), but it does not verify the branch where !type triggers fetchBookmarkData(url, body, 'bookmark'). Add a focused test around OEmbedService’s fallback logic to assert that an undefined oEmbed type resolves to bookmark data and still uses the standard favicon behavior, so this specific contract is locked down.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ghost/core/core/server/services/oembed/oembed-service.js`:
- Line 568: Add a regression test for the exact undefined-type fallback path in
oembed-service.js: the current coverage splits the behavior across
fetchOembedDataFromUrl() and fetchBookmarkData(), but it does not verify the
branch where !type triggers fetchBookmarkData(url, body, 'bookmark'). Add a
focused test around OEmbedService’s fallback logic to assert that an undefined
oEmbed type resolves to bookmark data and still uses the standard favicon
behavior, so this specific contract is locked down.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34c66b35-e43d-46e9-a29d-96dc5c1d1a14
📒 Files selected for processing (1)
ghost/core/core/server/services/oembed/oembed-service.js
The fallback path (!data && !type) now passes 'bookmark' so pickFn selects the standard favicon instead of the Apple Touch icon. The existing favicon tests only covered explicit type === 'bookmark', so the fallback path with an undefined type was uncovered. Also corrected the pickFn comment, which wrongly described the oembed fallback as a scaled-up tile preferring Apple Touch.
9larsons
force-pushed
the
fix/bookmark-favicon-fallback-type
branch
from
d30a80e to
6bf6124
Compare
|
@lujuldotcom Thanks for this - added a test to capture this which would've prevented this fix with the previous PR! |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 13s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 2m 43s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 56s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 21s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 29s | View ↗ |
nx run-many -t lint -p ghost |
✅ Succeeded | 35s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 6s | View ↗ |
Additional runs (2) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-01 14:10:28 UTC
2 participants
Hi @9larsons,
I tested the merged change and found that bookmark cards can still prefer Apple Touch Icons in the oEmbed fallback path.
The bookmark-specific favicon logic works when
fetchBookmarkDatais called withtype === 'bookmark', but the fallback path currently calls it with the original undefined type:That means pickFn does not enter the bookmark-specific branch and falls back to the previous Apple-first behavior.
This change passes
'bookmark'explicitly in that fallback path:data = await this.fetchBookmarkData(url, body, 'bookmark');